Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore fields that are annotated with the @MockBean and @SpyBean annotations of Spring test #757

Merged
merged 4 commits into from
Sep 20, 2023

Conversation

uhafner
Copy link
Contributor

@uhafner uhafner commented Apr 18, 2023

This is a small followup to #477. When you are testing Spring (Boot) applications then you are replacing controllers that are normally injected via @Autowired with stubs, mocks or spies from Mockito. This is done with two similar annotations:

  • @MockBean
  • @SpyBean

So it would be helpful if these are ignored as well.

@uhafner
Copy link
Contributor Author

uhafner commented Apr 18, 2023

Hmm, it seems that the dependency org.springframework.boot:spring-boot-test:2.7.5 causes the problem. I tried locally with 2.4.0, but that does not work as well. Is there a way to specify an exclusion in those dependencies? I tried with { exclude group: 'org.springframework.boot', module: 'spring-boot' } but that does not work in the test map.

@lazaroclapp
Copy link
Collaborator

Hmm, it seems that the dependency org.springframework.boot:spring-boot-test:2.7.5 causes the problem. I tried locally with 2.4.0, but that does not work as well. Is there a way to specify an exclusion in those dependencies? I tried with { exclude group: 'org.springframework.boot', module: 'spring-boot' } but that does not work in the test map.

Haven't had a chance to look at this in any level of detail, but one thing that seems to be happening is our existing tests failing with java.lang.NoClassDefFoundError. It is possible that spring-boot-test:2.7.5 is pulling in a more recent version of some common test dependency (e.g. mockito?) which we are using, which is no longer compatible with how we use it in tests.

@uhafner
Copy link
Contributor Author

uhafner commented Apr 19, 2023

Hmm, it seems that the dependency org.springframework.boot:spring-boot-test:2.7.5 causes the problem. I tried locally with 2.4.0, but that does not work as well. Is there a way to specify an exclusion in those dependencies? I tried with { exclude group: 'org.springframework.boot', module: 'spring-boot' } but that does not work in the test map.

Haven't had a chance to look at this in any level of detail, but one thing that seems to be happening is our existing tests failing with java.lang.NoClassDefFoundError. It is possible that spring-boot-test:2.7.5 is pulling in a more recent version of some common test dependency (e.g. mockito?) which we are using, which is no longer compatible with how we use it in tests.

spring-boot-test depends "only" on spring-boot and that causes the pain. This is the reason why I tried to use the exclude for spring boot which pulls in a lot of transitive dependencies. I'm almost sure that there is a conflict.

I'm a Maven user so I am not familiar with Gradle: in Maven you would simply exclude the transitive dependency, it should not be required during compile time for these two annotations. But the syntax in your scripts seems to not allow exclusions, at least I found no way to add one.

(The PR is not really important for me, I just thought it might make sense to add these annotations because I needed to add some suppressions in my code. So we should not spend too much time to fix it, if the testing framework does not support it out of the box.)

@msridhar
Copy link
Collaborator

Sorry for the long delay in looking at this. I don't think the transitive dependency is the issue. For the failing test I see org.springframework.boot.test.mock.mockito.SpringBootMockResolver.resolve at the top of the stack, and SpringBootMockResolver is inside spring-boot-test-2.7.5.jar. I think unfortunately there is some interaction between spring-boot-test and Mockito that makes our existing tests unhappy.

An alternative here is to just "stub out" the relevant @MockBean and @SpyBean annotations within NullAway itself, rather than taking the dependence. We already do this for a bunch of Android-related types. See here:

https://github.com/uber/NullAway/tree/1ff88b64e527cdb529f5df38ab5ade8dc01b0352/nullaway/src/test/resources/com/uber/nullaway/testdata/androidstubs

Could we take the same approach in this PR, so we don't even need the dep on spring-boot-test?

@uhafner
Copy link
Contributor Author

uhafner commented Jun 19, 2023

Thanks for the pointer! I'll look into those classes and try to refactor the PR in this way.

@msridhar
Copy link
Collaborator

Thanks for the pointer! I'll look into those classes and try to refactor the PR in this way.

Thanks! Note that the directory structure doesn't matter here, i.e., it doesn't need to match the package names. So I'd create a folder nullaway/src/test/resources/com/uber/nullaway/testdata/spring-boot-annotations, put stub source files for the two annotations there (with the correct package name), and then for the tests, add them as additional source files, as is done here:

public void supportLibFragmentMissingOnCreateError() {
initialiseAndroidCoreClasses();
compilationHelper
.addSourceFile("androidstubs/supportlib/Fragment.java")
.addSourceFile("android-error/SupportLibraryFragmentWithoutOnCreate.java")
.doTest();
}

@ChristianCiach
Copy link

@uhafner Are you still working on it? We are having the same issue and are currently workarounding it by configuring NullAway with..

-XepOpt:NullAway:ExcludedFieldAnnotations=org.springframework.boot.test.mock.mockito.(Mock|Spy)Bean

I know nothing about the internal architecture of NullAway, but if you're busy I may give @msridhar suggestions a shot.

@msridhar
Copy link
Collaborator

Thanks for fixing this up! This now LGTM assuming tests pass

@msridhar msridhar enabled auto-merge (squash) September 20, 2023 14:28
@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.08% 🎉

Comparison is base (70af259) 86.72% compared to head (e158336) 86.80%.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #757      +/-   ##
============================================
+ Coverage     86.72%   86.80%   +0.08%     
  Complexity     1870     1870              
============================================
  Files            74       74              
  Lines          6178     6178              
  Branches       1202     1202              
============================================
+ Hits           5358     5363       +5     
+ Misses          408      405       -3     
+ Partials        412      410       -2     
Files Changed Coverage Δ
...va/com/uber/nullaway/ErrorProneCLIFlagsConfig.java 87.20% <ø> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@msridhar msridhar merged commit ab387b9 into uber:master Sep 20, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants